node: Validate and sanitize RPC URLs#4822
Conversation
johnsaigle
commented
May 20, 2026
johnsaigle
commented
- Checks watcher's URLs against a deny-list of public RPC URLs. Logs an error when a public RPC URL is found for a delegated guardian. This should prevent problems where a delegate is not correct pointed at a full node.
- Sanitizes URLs in logs so that path parts are not included. (These can sometimes contain API keys or other sensitive strings).
- Normalizes logged field names across watchers for the 'watcher started' log
| } | ||
|
|
||
| // SafeURLForLogging returns only the hostname for a URL-like string. | ||
| // It intentionally omits userinfo, path, query, and fragment because those may contain credentials. |
There was a problem hiding this comment.
Why also strip the path? I don't think that would realistically contain any secrets plus would be helpful to know the actual path delimited endpoint in some scenarios?
There was a problem hiding this comment.
IMO we need it. e.g. for Alchemy, they give you a URL like this: wss://eth-mainnet.g.alchemy.com/v2/W_...<secret here>..., and that would be part of the path. I don't know why they do it like that, but we have to work around it.
There was a problem hiding this comment.
Is there any reason that we would WANT to log the path or query? If it's important enough, I could see a world where we only block/strip known patterns or use a regex to strip out specific portions of the path?
There was a problem hiding this comment.
I'm not aware of a reason right now. I think we can go ahead with this change for now and do more advanced parsing later if we need it. It seems like a bit of a headache to try to think of every possible provider and analyze their APIs to satisfy a regex or identify known-bad patterns.
|
@djb15 can you please re-review? I had to rebase and I also squashed the commits. Otherwise everything should be the same. |
djb15
left a comment
djb15
left a comment
There was a problem hiding this comment.
Verified the rebase didn't change anything drastic
- Checks watcher's URLs against a deny-list of public RPC URLs. Logs an error when a public RPC URL is found for a delegated guardian. This should prevent problems where a delegate is not correct pointed at a full node. - Sanitizes URLs in logs so that path parts are not included. (These can sometimes contain API keys or other sensitive strings). - Normalizes logged field names across watchers for the 'watcher started' log
| if err != nil || parsedURL.Host == "" { | ||
| // Schemeless host:port strings parse as scheme:opaque. Parse them again | ||
| // as network-path references so URL.Hostname can extract the host. | ||
| parsedURL, err = url.Parse("//" + urlStr) |
There was a problem hiding this comment.
Should we keep the scheme? I don't see the harm and it would make debugging easier.
| } | ||
|
|
||
| for _, rpcURL := range rpcURLs { | ||
| hostname := common.SafeURLForLogging(rpcURL) |
There was a problem hiding this comment.
The function is being used for things other than logging. Maybe worth considering a different name?
There was a problem hiding this comment.
What do you mean exactly? I think it's always used to format a string for a log or else format a string that goes into an error message that then gets logged.
| // It logs an error once per chain per run if a public RPC endpoint is detected. | ||
| // This is a safety measure: delegating guardians that use public RPC endpoints | ||
| // risk producing incorrect VAAs, which can cause consensus failures. | ||
| func (p *Processor) checkPublicRpcEndpoints() { |
There was a problem hiding this comment.
I believe this is still safe for the ReobserveWithEndpoint calls. It's worth a double check though.
I think that preventing guardians from using public RPCs is a good call.
| if err := p.dgc.Set(chains); err != nil { | ||
| p.logger.Error("delegate guardian config update failed", zap.Error(err)) | ||
| } else { | ||
| p.checkPublicRpcEndpoints() |
There was a problem hiding this comment.
Should this just log an error or just it exit loudly?
There was a problem hiding this comment.
If it exists here then I think it triggers a processor restart and crash loop. I think for the moment it's better to log and from there we can contract Guardians to figure out what's up
4 participants
